Initial Cluster Actuator Implementation #17
Conversation
Defines a top-level AzureClient interface to hold all clients and operations for creating and checking for existence of an NSG
This is now handled by the cluster actuator
| } | ||
|
|
||
| func (azure *AzureClusterClient) Delete(cluster *clusterv1.Cluster) error { | ||
| //TODO: get rid of the whole resource group? |
There was a problem hiding this comment.
would this be the right thing to do? Figuring out the dependencies in the cluster between virtual machines and the created NICs and SG would be a hassle. Would there be a case when the Delete operation shouldn't be tearing down all cluster resources?
There was a problem hiding this comment.
I think deleting the whole resource group sounds like a good initial implementation. Figuring out resource dependencies is a nightmare and I don't see a use case where a Delete operation shouldn't delete all cluster resources.
| if err != nil { | ||
| return err | ||
| } | ||
| networkSGFuture, err := azure.network().CreateOrUpdateNetworkSecurityGroup(clusterConfig.ResourceGroup, "ClusterAPINSG", clusterConfig.Location) |
There was a problem hiding this comment.
should move this to a const under securitygroups.go and default to that name if no SG name is provided.
Refer to that defined constant from machineactuator.go as well.
There was a problem hiding this comment.
Yes please. Even better if we have a module just for constants.
|
Hi @marwanad, I'll get back to you with a review later today. In the meantime, can you check your PR's Travis build: https://travis-ci.org/platform9/azure-provider/builds/442404123? |
marwanad
force-pushed
the
cluster-actuator-nsg
branch
from
198736b to
eef1efe
Compare
marwanad
force-pushed
the
cluster-actuator-nsg
branch
from
eef1efe to
25ece3b
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| networkSGFuture, err := azure.network().CreateOrUpdateNetworkSecurityGroup(clusterConfig.ResourceGroup, "ClusterAPINSG", clusterConfig.Location) |
There was a problem hiding this comment.
Yes please. Even better if we have a module just for constants.
| } | ||
|
|
||
| func (azure *AzureClusterClient) Delete(cluster *clusterv1.Cluster) error { | ||
| //TODO: get rid of the whole resource group? |
There was a problem hiding this comment.
I think deleting the whole resource group sounds like a good initial implementation. Figuring out resource dependencies is a nightmare and I don't see a use case where a Delete operation shouldn't delete all cluster resources.
| @@ -64,6 +65,12 @@ func (azure *AzureClusterClient) Reconcile(cluster *clusterv1.Cluster) error { | |||
| if err != nil { | |||
| return fmt.Errorf("error loading cluster provider config: %v", err) | |||
| } | |||
|
|
|||
| _, err = azure.resourcemanagement().CreateOrUpdateGroup(clusterConfig.ResourceGroup, clusterConfig.Location) | |||
There was a problem hiding this comment.
the resource group creation and updating should be part of the cluster reconciliation loop
marwanad
force-pushed
the
cluster-actuator-nsg
branch
from
978ee36 to
fa302b1
Compare
Deletes the cluster resource group
marwanad
force-pushed
the
cluster-actuator-nsg
branch
from
61e8dc1 to
be783ba
Compare
This moves the responsibility of creating and reconciling the NSG to the cluster actuator. (I'll be moving the VNet in a separate PR).
I tested the reconciliation to the SSH rule by deleting the rule through the portal and waiting for the cluster controller to reconcile.
Working towards #9